feat: transformer redesign #5534

yuth · 2020-10-08T19:06:02Z

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

lgtm-com · 2020-10-08T19:25:02Z

This pull request introduces 26 alerts when merging 1cb94b3 into c932d6f - view on LGTM.com

new alerts:

24 for Unused variable, import, function or class
1 for Useless assignment to local variable
1 for Unneeded defensive code

lgtm-com · 2020-10-08T19:55:40Z

This pull request introduces 26 alerts when merging c9a1460 into c932d6f - view on LGTM.com

new alerts:

24 for Unused variable, import, function or class
1 for Useless assignment to local variable
1 for Unneeded defensive code

codecov · 2020-10-08T19:58:14Z

Codecov Report

Merging #5534 (77132a2) into master (56f41f8) will decrease coverage by 0.16%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5534      +/-   ##
==========================================
- Coverage   58.15%   57.99%   -0.17%     
==========================================
  Files         407      410       +3     
  Lines       18747    18769      +22     
  Branches     3745     3750       +5     
==========================================
- Hits        10903    10885      -18     
- Misses       7162     7200      +38     
- Partials      682      684       +2

Impacted Files	Coverage Δ
...dejs-function-runtime-provider/src/utils/invoke.ts	`0.00% <0.00%> (-88.24%)`	⬇️
...es/amplify-appsync-simulator/src/velocity/index.ts	`63.63% <0.00%> (-11.81%)`	⬇️
...sync-simulator/src/schema/appsync-scalars/index.ts	`29.50% <0.00%> (-10.77%)`	⬇️
...ackages/amplify-codegen/src/commands/statements.js	`88.88% <0.00%> (-1.81%)`	⬇️
packages/amplify-codegen/src/commands/types.js	`92.00% <0.00%> (-1.75%)`	⬇️
packages/amplify-cli/src/domain/amplify-toolkit.ts	`1.53% <0.00%> (-1.47%)`	⬇️
packages/amplify-cli/src/index.ts	`49.03% <0.00%> (-1.43%)`	⬇️
...psync-model-plugin/src/visitors/appsync-visitor.ts	`94.88% <0.00%> (-0.90%)`	⬇️
...s/graphql-predictions-transformer/src/resources.ts	`90.00% <0.00%> (-0.25%)`	⬇️
packages/amplify-codegen/src/utils/index.js	`100.00% <0.00%> (ø)`
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56f41f8...7a62b6e. Read the comment docs.

lgtm-com · 2020-10-08T20:23:19Z

This pull request introduces 7 alerts when merging 215463c into c932d6f - view on LGTM.com

new alerts:

3 for Useless assignment to local variable
3 for Unused variable, import, function or class
1 for Unneeded defensive code

lgtm-com · 2020-10-09T01:51:47Z

This pull request introduces 8 alerts when merging aeada80 into 952a92e - view on LGTM.com

new alerts:

4 for Unused variable, import, function or class
3 for Useless assignment to local variable
1 for Unneeded defensive code

lgtm-com · 2020-10-09T14:48:13Z

This pull request introduces 8 alerts when merging fbb6784 into 952a92e - view on LGTM.com

new alerts:

4 for Unused variable, import, function or class
3 for Useless assignment to local variable
1 for Unneeded defensive code

lgtm-com · 2020-10-22T21:17:48Z

This pull request introduces 18 alerts when merging faa5e25 into 77132a2 - view on LGTM.com

new alerts:

16 for Unused variable, import, function or class
1 for Useless assignment to local variable
1 for Unneeded defensive code

lgtm-com · 2020-10-30T01:30:40Z

This pull request introduces 30 alerts when merging 77db0f1 into 7a806b2 - view on LGTM.com

new alerts:

28 for Unused variable, import, function or class
1 for Useless assignment to local variable
1 for Unneeded defensive code

lgtm-com · 2020-10-30T19:02:44Z

This pull request introduces 20 alerts when merging 019a3ec into c0d73a6 - view on LGTM.com

new alerts:

17 for Unused variable, import, function or class
1 for Useless assignment to local variable
1 for Unreachable statement
1 for Unneeded defensive code

lgtm-com · 2020-10-30T19:47:18Z

This pull request introduces 20 alerts when merging 9246df1 into 319a3ae - view on LGTM.com

new alerts:

17 for Unused variable, import, function or class
1 for Useless assignment to local variable
1 for Unreachable statement
1 for Unneeded defensive code

lgtm-com · 2020-10-30T20:23:04Z

This pull request introduces 5 alerts when merging 381d87d into 319a3ae - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class
1 for Useless assignment to local variable
1 for Unreachable statement
1 for Unneeded defensive code

lgtm-com · 2020-11-03T16:07:25Z

This pull request introduces 5 alerts when merging e379e6e into 6143e4b - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class
1 for Useless assignment to local variable
1 for Unreachable statement
1 for Unneeded defensive code

attilah

Good job on this huge chunk, I appreciate all the comments around the new code!

It looks good to me I have a bunch of nits, most of them around naming fix as you wish:

In general I would avoid using the 'New' prefix for types as these will be part of the 'public API'
A lot of filenames are not matching the class/interface inside. It would be nice to be concise in this to make it easier to locate things. Of course when multiple types/functions are inside it makes sense.
We should leverage Record type all around we've a plenty of places with [...]: ... members
transformer-resources.ts is an empty file
A lot of Todos are in the code it would be nice to see if they're planned to be addressed in this PR or future one?
for (let foo of -> for (const foo of in some places
Defination -> Definition at some places, not sure tagged all of those
JSON.parse and stringify to use JSONUtilities

One extra thing I would like be addressed is to get out compileSchema from awscloudformationprovider as it is making that project a bottleneck for package loading.

packages/amplify-cli-core/src/feature-flags/featureFlags.ts

packages/amplify-graphql-model-transformer/package.json

packages/amplify-provider-awscloudformation/src/utils/transfomer-feature-flag-adapter.ts

packages/amplify-graphql-transformer-interfaces/src/graphql-api-provider.ts

packages/amplify-provider-awscloudformation/src/graphql-transformer/transform-config.ts

packages/amplify-provider-awscloudformation/src/graphql-transformer/utils.ts

packages/amplify-provider-awscloudformation/src/graphql-transformer/transform-graphql-schema.ts

lgtm-com · 2020-11-04T00:56:19Z

This pull request introduces 4 alerts when merging 941f862 into 955022c - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class
1 for Useless assignment to local variable
1 for Unneeded defensive code

packages/amplify-graphql-model-transformer/package.json

packages/amplify-graphql-model-transformer/src/graphql-model-transformer.ts

packages/amplify-provider-awscloudformation/src/graphql-transformer/transform-config.ts

packages/amplify-provider-awscloudformation/src/graphql-transformer/utils.ts

packages/amplify-provider-awscloudformation/src/graphql-transformer/transform-graphql-schema.ts

lgtm-com · 2020-11-04T22:52:10Z

This pull request introduces 2 alerts when merging 78e7730 into 955022c - view on LGTM.com

new alerts:

1 for Useless assignment to local variable
1 for Unneeded defensive code

packages/amplify-graphql-model-transformer/src/graphql-model-transformer.ts

packages/amplify-graphql-transformer-core/src/cdk-compat/nested-stack.ts

packages/amplify-graphql-transformer-core/src/cdk-compat/root-stack.ts

packages/amplify-graphql-transformer-core/src/transformation/transformer-plugin-base.ts

packages/amplify-provider-awscloudformation/src/graphql-transformer/transform-config.ts

lgtm-com · 2020-11-05T02:26:11Z

This pull request introduces 2 alerts when merging 72466cd into 955022c - view on LGTM.com

new alerts:

1 for Useless assignment to local variable
1 for Unneeded defensive code

lgtm-com · 2020-11-05T02:56:49Z

This pull request introduces 1 alert when merging e487512 into 955022c - view on LGTM.com

new alerts:

1 for Unneeded defensive code

lgtm-com · 2020-11-05T19:01:43Z

This pull request introduces 1 alert when merging 21b8ba0 into 955022c - view on LGTM.com

new alerts:

1 for Unneeded defensive code

lgtm-com · 2020-11-05T19:46:00Z

This pull request introduces 1 alert when merging 7ca5fd8 into 955022c - view on LGTM.com

new alerts:

1 for Unneeded defensive code

lgtm-com · 2020-11-05T21:22:53Z

This pull request introduces 1 alert when merging faf8bdf into 955022c - view on LGTM.com

new alerts:

1 for Unneeded defensive code

attilah · 2020-11-06T16:48:34Z

packages/amplify-graphql-transformer-core/src/transformation/transform.ts

+      throw new SchemaValidationError(errors);
+    }
+
+    // // check if the project is sync enabled


Is it leftover?

attilah

Added some new nit comments, but LGTM, we can enhance, change things later.

ammarkarachi · 2020-11-06T18:25:31Z

@yuth @attilah Can we merge this?

…r each resolver

New experimental graphql transformer with support for Pipeline functions. Currently this is behind a feature flag `useExperimentalPipelinedTransformer` and support only model directive.

github-actions · 2021-11-08T01:45:28Z

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels for those types of questions.

yuth requested a review from attilah October 8, 2020 19:06

yuth force-pushed the transformer-redesign branch from 1cb94b3 to c9a1460 Compare October 8, 2020 19:40

yuth force-pushed the transformer-redesign branch from 215463c to aeada80 Compare October 9, 2020 01:37

yuth force-pushed the transformer-redesign branch from aeada80 to fbb6784 Compare October 9, 2020 14:33

yuth force-pushed the transformer-redesign branch from fbb6784 to faa5e25 Compare October 22, 2020 21:01

yuth force-pushed the transformer-redesign branch from faa5e25 to 77db0f1 Compare October 30, 2020 01:14

yuth force-pushed the transformer-redesign branch from 6859a16 to 103d0b0 Compare October 30, 2020 19:20

attilah reviewed Nov 3, 2020

View reviewed changes

edwardfoyle reviewed Nov 4, 2020

View reviewed changes

yuth force-pushed the transformer-redesign branch from 9a70715 to 78e7730 Compare November 4, 2020 22:33

yuth commented Nov 5, 2020

View reviewed changes

yuth marked this pull request as ready for review November 5, 2020 18:47

attilah reviewed Nov 6, 2020

View reviewed changes

attilah approved these changes Nov 6, 2020

View reviewed changes

yuth added 19 commits November 6, 2020 15:32

feat: transformer redesign

fb47412

Custom graphql api

09a66d4

wip

3cca1b4

wip

347ec64

wip

0ef2ff9

add support for inline and s3 mapping template and expose metadata fo…

c2d5791

…r each resolver

checkpoint - need to ad list, sync and subscription resolvers

8d1822f

all resolver working

66f6ba8

package version bumps

84e3268

fix lint errors

e3a02df

add assertion

90123e9

bump the depenscy package version

ad3797b

update linter rule

5b36d48

fix build error

c08c020

handle subscription type in get args

7857b97

WIP

d43ae8f

address review comments

3266142

change resolver var name

cbd0924

review comments

7a62b6e

yuth force-pushed the transformer-redesign branch from faf8bdf to 7a62b6e Compare November 6, 2020 23:45

yuth merged commit a93c685 into aws-amplify:master Nov 7, 2020

github-actions bot locked as resolved and limited conversation to collaborators Nov 8, 2021

feat: transformer redesign #5534

feat: transformer redesign #5534

Uh oh!

Conversation

yuth commented Oct 8, 2020

Uh oh!

lgtm-com bot commented Oct 8, 2020

Uh oh!

lgtm-com bot commented Oct 8, 2020

Uh oh!

codecov bot commented Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lgtm-com bot commented Oct 8, 2020

Uh oh!

lgtm-com bot commented Oct 9, 2020

Uh oh!

lgtm-com bot commented Oct 9, 2020

Uh oh!

lgtm-com bot commented Oct 22, 2020

Uh oh!

lgtm-com bot commented Oct 30, 2020

Uh oh!

lgtm-com bot commented Oct 30, 2020

Uh oh!

lgtm-com bot commented Oct 30, 2020

Uh oh!

lgtm-com bot commented Oct 30, 2020

Uh oh!

lgtm-com bot commented Nov 3, 2020

Uh oh!

attilah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgtm-com bot commented Nov 4, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgtm-com bot commented Nov 4, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgtm-com bot commented Nov 5, 2020

Uh oh!

lgtm-com bot commented Nov 5, 2020

Uh oh!

lgtm-com bot commented Nov 5, 2020

Uh oh!

lgtm-com bot commented Nov 5, 2020

Uh oh!

lgtm-com bot commented Nov 5, 2020

Uh oh!

attilah Nov 6, 2020

Choose a reason for hiding this comment

Uh oh!

attilah left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 8, 2020 •

edited

Loading